feat(migration): move onboarding import into Berd - #202
Conversation
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Fresh static review of exact range 20c005a...a627092 found 2 blocking findings and 0 non-blocking findings. Final self-check covered the complete first-boot import flow (backup, fixed-location discovery, config/secrets/extensions/skills import, provider defaults, extension disabling, migration completion and retry), accessibility and i18n (no new UI), navigation/consent guards, async timeout/error/never-completes/lifecycle/race behavior, test honesty, project laws and design-system rules, duplicate overlap, and concrete user effect. Supplied GitHub evidence was inspected: all reported head-SHA check runs completed successfully except the skipped close job; the combined commit status remained pending, so required checks still independently govern merge readiness.
Deterministic publication result: 2 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
| } | ||
| } | ||
| if changed { | ||
| let _ = send_acp_request( |
There was a problem hiding this comment.
🤖 P1 · Preserve concurrent extension changes (blocking)
The import reads the entire current extensions object, merges imported entries into that snapshot, and then replaces the whole object with a later upsert. Any other Goose client or app instance that adds, edits, or removes an extension between the read and this write is silently overwritten. This is a persisted-config migration, so the snapshot-then-write race can lose a concurrent writer's saved intent.
User effect: A connection changed in another Berd window or Goose client during first-run migration can unexpectedly revert or disappear.
Recommended fix: Avoid replacing a stale full-map snapshot. Apply each missing imported extension through an atomic Goose extension-add/patch operation, or add a backend compare-and-set/merge operation that re-reads and verifies immediately before committing and retries on conflict.
Test: Add a discriminating ACP test that changes the server's extensions after the import read but before the import write, then assert both the concurrent change and imported extension survive.
| "Skipped Goose configuration import at '{}': {error}", | ||
| path.display() | ||
| )); | ||
| continue; |
There was a problem hiding this comment.
🤖 P1 · Import sibling data independently (blocking)
A missing, unreadable, oversized, or malformed config.yaml immediately continues the loop, so the same legacy directory's valid secrets.yaml and skills directory are never examined. The command still returns a warning, runMigration only writes it to the developer console, and the caller can mark migration complete, turning this recoverable source-file problem into a permanent skipped import. The existing malformed-extension test does not cover malformed top-level config.
User effect: A user with a damaged or absent legacy config can silently lose automatic migration of otherwise valid credentials and personal skills, with no in-product indication or later retry.
Recommended fix: Treat config, secrets, and skills as independent artifacts: record the config warning but continue scanning sibling secrets and skills. Ensure skipped artifacts remain retryable or surface a user-visible, localized recovery notice before finalizing the one-shot migration.
Test: Add a migration test with invalid (and separately absent) config.yaml beside valid secrets.yaml and a valid skill; assert the secret update and skill import still occur and that recoverable failures are not silently finalized.
Address the parity findings from the PR #202 review, restoring behavior the removed Goose onboarding importer had: - Treat a non-mapping config.yaml top level (empty file, null, scalar) as an empty mapping so secrets.yaml and legacy skills still import for that root; invalid YAML still skips the candidate as before. - Copy Claude Desktop MCP server env maps verbatim instead of filtering keys through a deny list the old importer never had. - Sweep stale .berd-onboarding-* staging directories at the start of copy_legacy_skills so a crash between copy and publish rename cannot leave orphaned entries behind forever. Left as-is per the pre-existing-behavior rule: all-or-nothing secrets.yaml parsing and the empty extension config key produced by a name with no ASCII alphanumerics, both of which match the old importer's name_to_key/apply behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Fresh static review of exact range 20c005a...11929da found 2 new blocking findings and 0 new non-blocking findings; 2 still-present findings were suppressed because they map to existing unresolved automation threads. The final self-check covered the complete first-boot flow (backup, fixed-location discovery, config/secrets/extensions/skills import, provider defaults, extension disabling, marker completion and retry), accessibility and localization (no new UI), navigation and consent guards, async error/timeout/never-completes/lifecycle/race behavior, test honesty, project laws and design-system rules, overlap/duplicate handling, and concrete evidence plus user effect for blocking severity. Supplied GitHub evidence was structurally valid and inspected: clean-room install and DCO passed, while frontend and platform Rust checks were still in progress; required checks independently govern merge readiness.
Deterministic publication result: 2 blocking and 0 non-blocking finding(s) publishable; 2 duplicate(s) suppressed.
| if !entry | ||
| .file_name() | ||
| .to_string_lossy() | ||
| .starts_with(SKILL_STAGING_PREFIX) |
There was a problem hiding this comment.
🤖 P1 · Do not delete prefix-matched skills (blocking)
The new retry cleanup treats every directory in the user's personal skills root whose name starts with .berd-onboarding- as disposable staging and recursively deletes it. There is no ownership marker, recorded staging path, age check, or exact generated-name validation. A legitimate user directory with that prefix is therefore destroyed, and a second Berd instance can also delete a staging directory while the first instance is actively copying it. This is destructive migration behavior based only on a filename resemblance.
User effect: On first boot or a migration retry, a user can permanently lose a personal skill directory, or a concurrent import can be corrupted, without any confirmation or recovery path.
Recommended fix: Track staging directories created by this migration with verifiable ownership metadata and clean up only directories proven stale and owned by Berd. Prefer a dedicated private staging parent outside the user-visible skills namespace, and coordinate concurrent import attempts before deleting anything.
Test: Add a discriminating test that places a user-authored .berd-onboarding-custom skill in the target root and asserts it survives cleanup; also cover two concurrent staging owners so one run cannot remove the other's active directory.
|
|
||
| let mut next_id = 2; | ||
| for update in secret_updates { | ||
| let _ = send_acp_request( |
There was a problem hiding this comment.
🤖 P1 · Bound the whole startup import (blocking)
The migration performs as many as 256 secret writes serially and gives every request its own 15-second deadline. Because this command sits on the first-boot migration gate, a slow but responsive Goose peer can keep startup unsettled for roughly 64 minutes before extension work even begins. The per-request timeout prevents one request from waiting forever, but it does not provide a degraded path for the overall operation; the comment explicitly chooses repeated deadlines, and the delayed-peer test only proves that this long-running behavior succeeds.
User effect: A user can be unable to reach a ready Home chat for many minutes—or close to an hour—while a silent import waits on sequential backend writes.
Recommended fix: Remove network import work from the startup-critical path, or enforce a short aggregate presentation deadline that lets the app become usable while bounded background work continues. Preserve retry progress so an aggregate deadline does not repeatedly restart at the same entry.
Test: Add a fake-clock integration test with many just-under-timeout responses and assert the migration gate reaches a usable degraded state within the product-level startup budget while remaining able to resume safely.
2 participants
Summary
Related issue
aaif-goose/goose#11451
Testing
just checkjust tauri-checkjust clippyjust _tauri-cargo-unix test --lib commands::onboarding_import::tests(16 passed)pnpm vitest run src/features/migration/runMigration.test.ts(passed)just test(7021 passed, 1 skipped; one unrelated telemetry/localStorage test fails in this environment)just _tauri-cargo-unix test --lib(828 passed; seven unrelated base-branch failures: five layout default-zoom assertions and two Goose runtime-provider fixture assertions)No UI layout or visual behavior changes.